Block editor: add a way to extend style dependency handles for editor content #37466
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ellatrix
requested review from
ajitbohra,
nerrad,
ntwb,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: +351 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
priethor
added
[Feature] Full Site Editing
[Package] Block editor
ellatrix
force-pushed
the
try/pass-style-handle-through-block-editor-settings
branch
from
8527e4f to
9f48ff1
Compare
oandregal
reviewed
Dec 23, 2021
|
|
||
| sheet.deleteRule( index ); | ||
| sheet.insertRule( | ||
| 'html :where(.editor-styles-wrapper) ' + cssText, |
There was a problem hiding this comment.
Hey, I don't understand why we're prefixing the styles with html :where(.editor-styles-wrapper) instead of .editor-styles-wapper.
There was a problem hiding this comment.
If I'm not mistaken I think it's just to lower the specificity, but I'm also curious to know if that is indeed the case or not 🤔
There was a problem hiding this comment.
Yeah, I understand it lowers the specificity but I don't know why. I guess my question is: why the styles loaded through a wp dependency should be any different than the styles loaded via add_editor_styles?
There was a problem hiding this comment.
I just used the same thing as #32659. But I guess if we consistently increase the specificity of all styles, this is not necessary. Cc @jasmussen.
There was a problem hiding this comment.
Where did we land in this conversation? I'd think we want to use the same we use for styles passed via add_editor_style?
There was a problem hiding this comment.
Adding the :where will ensure that the styles that are prefixed with .editor-styles-wrapper have the same specificity in the editor as they have in the frontend.
oandregal
reviewed
Dec 23, 2021
| function PrefixedStyle( { tagName, href, id, rel, media, textContent } ) { | ||
| const TagName = tagName.toLowerCase(); | ||
|
|
||
| function onLoad( event ) { |
There was a problem hiding this comment.
This technique is cool, though there's a noticeable delay in the styles being applied. Presumably, because we add the stylesheet in the client after all things have been already loaded and the onLoad event happens even after.
Thinking about how to improve this:
- What if we convert all styles (both
linkandstyle) to embedded styles (style) and prefixed them just here, so we don't wait toonLoadto do it? - Alternatively, would it make sense to add the stylesheets in the server (no passing them through
__unstableResolvedContentStyles) and attach to itsonLoadevent a public function that takes care of prefixing them? While we still rely on theonLoadevent here it'll happen earlier because we're not adding it in the client.
There was a problem hiding this comment.
The good thing here is that we don't have to do any CSS parsing. I talked with @mcsf about the delay. I wonder if we could block the original CSS from applying, maybe by emptying and later refilling it. 🤔
It would be good if we don't have to do any parsing. I was hoping there would be some API to scope link and style tags to a given selector, but that doesn't exist apparently.
There was a problem hiding this comment.
This would be ideal: https://css-tricks.com/saving-the-day-with-scoped-css/
There was a problem hiding this comment.
@oandregal Could you remind me where you see the delay?
I now removed the prefixing from the iframes since it's not needed there. Only the "old" non iframe editors will need prefixing.
There was a problem hiding this comment.
I was able to repro with a big post (Gutenberg demo). It's more noticeable the 1st time the post is loaded and if you disable the cache. Also by throtling the network and/or the CPU it becomes more visible.
There was a problem hiding this comment.
What is more visible? What do you see? Could you record it?
mcsf
reviewed
Dec 23, 2021
packages/e2e-tests/specs/editor/plugins/iframed-editor-style.test.js
Outdated
Show resolved
Hide resolved
ellatrix
force-pushed
the
try/pass-style-handle-through-block-editor-settings
branch
from
9f48ff1 to
4aa9fe4
Compare
ellatrix
force-pushed
the
try/pass-style-handle-through-block-editor-settings
branch
from
4aa9fe4 to
251b6c9
Compare
This was referenced Jan 13, 2022
|
So I'm not sure why two e2e tests are failing. They're passing locally and they were passing before the last PR. 🤔 |
|
@ellatrix, it looks like all checks are passing now. |
ellatrix
force-pushed
the
try/pass-style-handle-through-block-editor-settings
branch
2 times, most recently
from
ea821aa to
42d23e7
Compare
oandregal
reviewed
Aug 31, 2022
| @@ -77,6 +141,22 @@ export default function EditorStyles( { styles } ) { | |||
| { /* Use an empty style element to have a document reference, | |||
| but this could be any element. */ } | |||
| <style ref={ useDarkThemeBodyClassName( styles ) } /> | |||
| { _styles.map( | |||
There was a problem hiding this comment.
Where do you think these styles should load? Before or after the other inline styles? I'd think they should load after.
I've also found that styles are now inconsistent, they load in different order depending on the context.
In the front, post editor, and template editor we do:
- inline global styles
- theme styles (the ones loaded via add_editor_style() in the editors)
In the site editor we do:
- theme styles
- global styles
cc @scruffian @andrewserong I thought we had them all loading in the same order?
There was a problem hiding this comment.
Sure, we can load them after.
|
@ellatrix I've read through the issues and took a quick look at what's the goal here. I understand that we want:
At the moment, the way they can do it is (essentially bypass the add_action(
'block_editor_settings_all',
function( $settings ) {
$settings['styles'][] = array( 'css' => 'p { border: 1px solid red }' );
return $settings;
}
);Is this correct? I like that this PR adds the stylesheets instead of the embedded styles. Though in terms of API ergonomics, for 3rd parties, using the new AlternativesI presume you've thought about other alternatives and I've probably missed those conversations. Thanks for bearing with me :) These are the alternatives I can think of:
|
|
@oandregal Tbh, at the moment, this is at first for us to use. See #35950. Sure, it's also an advanced way for themes and plugins to add handles if there's no other way. Ideally themes and plugins should all be able to add handles through block.json and theme.json, but there's some use cases where you can't use these (like for our packages). The API is currently a bit verbose, but it's meant to be experimental. We can always wrap it in something like And yes, ideally the embedded CSS is replaced entirely with dependency handles (either through this API, block.json or theme.json). |
ellatrix
force-pushed
the
try/pass-style-handle-through-block-editor-settings
branch
from
b00bafa to
280fe06
Compare
oandregal
reviewed
Sep 5, 2022
ellatrix
force-pushed
the
try/pass-style-handle-through-block-editor-settings
branch
from
280fe06 to
b4b1eac
Compare
annezazu
added
[Feature] Site Editor
ellatrix
deleted the
try/pass-style-handle-through-block-editor-settings
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
#35950 but without using it for internal styles. This PR adds the possibility to add a WP style dependency handle to the block editor settings, which will be resolved afterwards and added as content styles. If the editor is not iframed, the styles will be prefixed.
An example can be found in the e2e test:
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).